Fix coverage blocks overlapping #3575
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3575 +/- ##
==========================================
- Coverage 85.79% 85.35% -0.45%
==========================================
Files 330 333 +3
Lines 38536 39006 +470
==========================================
+ Hits 33062 33292 +230
- Misses 3928 4144 +216
- Partials 1546 1570 +24 ☔ View full report in Codecov by Sentry. |
Slava0135
force-pushed
the
fix-coverage-overlap
branch
from
71bd0be
to
7729e65
Compare
|
Fixed issues and made it a single commit |
Slava0135
force-pushed
the
fix-coverage-overlap
branch
from
7729e65
to
794f204
Compare
| ) | ||
|
|
||
| func resetCoverage() { | ||
| rawCoverage = make(map[util.Uint160]*scriptRawCoverage) |
There was a problem hiding this comment.
This map is protected by mutex, so at least we need to use mutex. Also, these tests prohibit NeoGo from using contract coverage in the project; we either need to document this or to change the test structure to skip them if contract coverage is enabled.
There was a problem hiding this comment.
We could make new map, save old map and then restore original map in Cleanup
There was a problem hiding this comment.
It won’t work if tests run in parallel.
| continue | ||
| } | ||
| // outer interval start must be before inner interval start. | ||
| if !(outer.StartLine < inner.StartLine || outer.StartLine == inner.StartLine && outer.StartCol <= inner.StartCol) { |
There was a problem hiding this comment.
The problem with this algorithm is that we can't just remove all outer intervals, otherwise coverage for some parts of code is lost and the result looks strange a bit. Consider neofs-contracts as an example, here's the result with the old implementation:
And here's the result with the overlaps removal:
return runtime.CheckWitness( is an outer interval and it's just removed.
There was a problem hiding this comment.
Also, I'm not quite sure why the first call to runtime.CheckWitness is marked as not covered, although it should be.
There was a problem hiding this comment.
Also, I'm not quite sure why the first call to
runtime.CheckWitnessis marked as not covered, although it should be.
Pretty sure, its because there is no SeqPoint for it
There was a problem hiding this comment.
I have checked how Go itself handles statement coverage and it seems like it takes a line as a whole (except for conditions where paths can diverge, though still counts for outer function if inner function panics). So we could keep outer block if its one the same line as inner (both are one-liners), but remove outer block if its multi-line?
There was a problem hiding this comment.
Could you give an example of contract with test where coverage blocks are overlapping?
There was a problem hiding this comment.
PutNamed function in container contract.
|
In |
Signed-off-by: Slava0135 <[email protected]>
Slava0135
force-pushed
the
fix-coverage-overlap
branch
from
794f204
to
650f9a3
Compare
We firstly need to check the debug info for this contract, and if debug info is really missing these sequence points then it’s a separate issue. |
Problem
Resolves #3559.
Solution
Remove block if it includes any other block.